feat: add sticky PR drift diff comments, Node 24 runtime, and CI fixes - #25

Code Review: Principal Software Engineer & Security Researcher

Reviewed the changes in src/main.js, action.yml, tests, and CI updates for critical issues across security, correctness, performance, reliability, and architecture. The implementation is generally solid: version inputs are strictly validated and passed as positional arguments (avoiding shell injection), tokens are masked via core.setSecret, fork 403s on comment posts are handled gracefully as warnings, drift reports are posted before failing the step, and Node 24 + explicit @actions/exec are good upgrades.

Findings

• [Severity: High] — Security & Vulnerabilities

  • Location: run() → install via exec.exec('/bin/bash', ['-lc', 'curl -fsSL ${INSTALL_SCRIPT_URL} | sh ...'])
  • Problem: The action downloads and executes a remote shell script from https://raw.githubusercontent.com/pbufio/pbuf-registry/main/scripts/install.sh without pinning a commit SHA or verifying a checksum. A compromise of the main branch (or a MITM on the raw endpoint) grants arbitrary code execution on every consumer's runner. This is a classic supply-chain risk for GitHub Actions that install CLIs.
  • Fix: Prefer downloading a versioned binary (or the script at a specific commit) and verifying it against a published checksum/SHA256. Example sketch:

    // Prefer direct binary download + checksum verification
    // or pin the install script:
    const INSTALL_SCRIPT_URL = 'https://raw.githubusercontent.com/pbufio/pbuf-registry/<commit-sha>/scripts/install.sh'
    // + curl the script to a temp file, verify hash, then execute

• [Severity: Medium] — Correctness & Edge Cases / Reliability

  • Location: upsertPullRequestComment() — comment listing loop (for (let page = 1; page <= 5; page += 1))
  • Problem: Only the first 500 comments are scanned for the sticky marker. On very active PRs the existing comment can be missed, leading to duplicate sticky comments. Also, a transient API failure during listing aborts the whole upsert (warning only).
  • Fix: Use the GitHub Search API for comments containing the marker, or continue pagination until the marker is found or the list ends, with a higher cap and better retry/backoff. Example:

    // Prefer: GET /search/issues?q=repo:owner/repo+is:pr+commenter:app/pbuf-setup-action+"<!-- pbuf-drift-diff -->"
    // or paginate fully with Link header parsing and a max of ~20 pages

• [Severity: Medium] — Reliability & Error Handling

  • Location: runDriftDiff() and install exec calls
  • Problem: No explicit timeout is set on @actions/exec or the fetch calls (beyond the 10s AbortSignal). A hung pbuf process or slow registry can stall the job until the runner timeout.
  • Fix: Pass timeout (ms) to exec.exec options and consider a slightly higher AbortSignal for API calls under load.

• [Severity: Low] — Architecture & Maintainability

  • Location: runDriftDiff() — against and working_directory inputs
  • Problem: against is interpolated into a CLI argument without format validation. Safe today because arguments are passed as an array (no shell), but a malformed tag could produce confusing CLI errors. working_directory is used as cwd with no existence or path-safety check.
  • Fix: Add a simple regex/allow-list for against (similar to VERSION_RE) and optionally fs.existsSync + realpath check for the working directory before running.

No critical injection, auth bypass, or data-exposure issues were found. Secrets masking, input validation on version/fail_on, and the sticky-comment design (with truncation) are well done. Tests cover the happy path, invalid versions, and comment create/update flows.

Go / No-Go Verdict

Conditional Go. The code is safe to deploy for most users, but the unpinned remote install script is a material supply-chain risk that should be addressed (or explicitly accepted with a documented trust boundary) before wide adoption. The pagination limit is the next practical reliability improvement. Otherwise the feature is cleanly implemented and the CI/runtime upgrades are welcome.

Automated rigorous review focused on critical issues only.